Copy land increments and land DA confs to COM #1797

jiaruidong2017 · 2023-08-12T17:35:58Z

The NCEP 'dump' convention is the opposite of the JEDI IODA convention. For example, NCEP is [t03z-t09z), whereas IODA is (t03z-t09z], so the observations right at 03:00 and 09:00 will be missing (https://github.com/JCSDA-internal/ufo/issues/2233). This PR solved this issue by shifting the DA window_begin 1 second backward.
The above DA window manipulation is being handled somewhere else.

This PR created COM_CONF_TMPL, copied the generated YAML file from initialize to the COM and copied the increment files to the COM.

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes need updates to the documentation. I have made corresponding changes to the documentation
My changes generate no new warnings
New and existing tests pass with my changes
Any dependent changes have been merged and published

Copy the generated YAML file from initialize to the COM Copy the increment files to the COM/

github-actions · 2023-08-12T17:36:39Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

github-actions · 2023-08-12T17:39:57Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

CoryMartin-NOAA · 2023-08-14T14:45:32Z

@jiaruidong2017 did you test this? I was under the impression that for many cases the background has to be at the center of the window... I forget how the MetOffice gets around this issue, but they do something.

jiaruidong2017 · 2023-08-14T15:03:45Z

@CoryMartin-NOAA Yes, I did the test runs in the global-workflow, and it worked perfectly. The updated window setup did pick up the lost observations.

As I remembered, the MetOffice used the same approach by moving the DA window backward 1 second. What they did is to do this shift in the yaml file. However, their approach only works in the HofX, but not in the letkf.x.

CoryMartin-NOAA · 2023-08-14T15:05:35Z

@jiaruidong2017 ok thanks, just wanted to double check.

CoryMartin-NOAA · 2023-08-14T20:11:38Z

ush/python/pygfs/task/land_analysis.py

@@ -34,6 +34,7 @@ def __init__(self, config):

        _res = int(self.config['CASE'][1:])
        _window_begin = add_to_datetime(self.runtime_config.current_cycle, -to_timedelta(f"{self.config['assim_freq']}H") / 2)
+        _window_begin = add_to_datetime(_window_begin, -to_timedelta(f"1S"))


to do this "correctly, now don't we need to make the window_length 6hours + 1 second?

@CoryMartin-NOAA I conducted and compared two runs: one with window length 6 hours and the other with 6 hours + 1 second. They do show some difference from the land increment files as below:

Tile 1: Variable Group Count Sum AbsSum Min Max Range Mean StdDev snodl / 432 0.00534215 0.00534219 -4.60635e-09 0.00056797 0.000567975 1.23661e-05 6.58902e-05 Tile 2: Variable Group Count Sum AbsSum Min Max Range Mean StdDev snodl / 1213 -0.570553 65.0728 -3.67096 3.36201 7.03297 -0.000470365 0.306991 Tile 3: Variable Group Count Sum AbsSum Min Max Range Mean StdDev snodl / 982 nan nan -94.825 98.1387 192.964 nan nan vtype / 105 nan nan inf -inf -inf nan nan slmsk / 105 nan nan inf -inf -inf nan nan Tile 4, 5, 6 are identical.

Let's talk at tomorrow's meeting.

@CoryMartin-NOAA @barlage When I conducted the snow DA cycling run with window length 6 hours + 2 seconds which make the DA window centering at the cycle time, all the nan values were gone. Therefore, the DA window centering at the cycle time is important.

CORRECTIONS:

My above statements are incorrect, because there are still nan values when the above setting with the DA window beginning at 1 second back and window length 6 hours + 2 seconds.

The above with no nan values came from the experiment with the DA window beginning at 2 second back and window length 6 hours + 1 seconds.

Since @CoryMartin-NOAA questioned in the meeting earlier, we looked at an increment file and not all of the grids are NaN, only about 100-200 and they look to be in locations where there are snow observations, which can seen from the above nccmp. Also, there must be some increment limits in the add_increment code since these do not affect the analysis.

@CoryMartin-NOAA Okay, I will change the window length to 6hr 1sec.

I don't understand, if the results are the same for 6hr, 6hr-1s, 6hr+1s, then why make the change?
Also, shouldn't this be handled in the observation processing part rather than in the workflow?
Consider the case where the observations are in a single file for 24h (for e.g.), shouldn't this be teased out in the yaml?

@aerorahul this is because the 'JEDI' window convention is the opposite of the 'NCEP' window convention, and we lost that battle with Yannick a while ago. While it may be identical in this case, it shouldn't be. Observations at exactly 03z would not make it into the 06z analysis without this change. I don't quite understand you comment in the second sentence, this is what defines the window variable that goes into the templated YAML.

I understand. Lets discuss offline.

Thanks @CoryMartin-NOAA for the detailed explanations.
@aerorahul Without this change, about several hundreds of observations will be lost in each cycle. For example, the observations increased from 1718 to 2161 with this changes.

CoryMartin-NOAA · 2023-08-14T20:11:54Z

ush/python/pygfs/task/land_analysis.py

@@ -336,6 +339,15 @@ def finalize(self) -> None:
        statfile = os.path.join(self.task_config.COM_LAND_ANALYSIS, f"{self.task_config.APREFIX}landstat.tgz")
        self.tgz_diags(statfile, self.task_config.DATA)

+        logger.info("Copy full YAML  to COM")


too many spaces

Removed the space

github-actions · 2023-08-15T01:13:36Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

centering at the cycle time.

github-actions · 2023-08-15T17:10:06Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

github-actions · 2023-08-15T17:17:00Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

github-actions · 2023-08-15T20:18:01Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

github-actions · 2023-08-16T13:58:49Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

github-actions · 2023-08-16T14:05:40Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

aerorahul · 2023-08-16T14:21:28Z

ush/python/pygfs/task/land_analysis.py

+        logger.info("Copy full YAML to COM")
+        src = os.path.join(self.task_config['DATA'], f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")
+        dest = os.path.join(self.task_config.COM_LAND_ANALYSIS, f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")
+        yaml_copy = {
+            'mkdir': [self.task_config.COM_LAND_ANALYSIS],
+            'copy': [[src, dest]]
+        }
+        FileHandler(yaml_copy).sync()


Why?
COM is for data not configuration. The UFS-weather-model has set a precedent for copying namelists and configuration stuff into COM. If there is a need to copy this data for every cycle, we need a proper solution with a well defined set of requirements.

Why not? This is a few kb file that is a record of the exact configuration needed to reproduce results. Especially in the land case, this is different at 18z than it is at other cycles. When observations and QC have to be turned on/off these YAMLs will not always be identical and should be retained.

Thanks @CoryMartin-NOAA. In addition, I include copying the YAML files for archive to keep consistent with other DA component such as aero DA.

An update to this comment:

After discussions w/ NCO, if we wish to retain cycle specific configuration artifacts such as namelists, etc. that are not already part of HOMEgfs, we can do so in a single location in COM. @CoryMartin-NOAA and @aerorahul discussed a good place for said configuration artifacts would be ROTDIR/RUN.PDY/cyc/conf.
The jobs will need to ensure that they identify the artifacts uniquely; for e.g. input.nml from the ufs-weather-model should be saved as ufs_input.nml

This will require creating a COM_CONF_TMPL in config.com and then filling the template with appropriate substitutions in the j-jobs that wish to save the artifacts.

Please let me know if you have any questions.
@WalterKolczynski-NOAA FYI

Thanks @aerorahul for the updates.
@CoryMartin-NOAA @aerorahul Do you want me to include this change in this PR by putting the yaml file into the ROTDIR/RUN.PDY/cyc/conf?

As suggested, I create COM_CONF_TMPL in config.com and copy the YAML file into ROTDIR/RUN.PDY/cyc/conf. I still keep using the previous name (e.g., gdas.t18z.letkfoi.yaml). What do you suggest the change for the file name?

ush/python/pygfs/task/land_analysis.py

aerorahul

Please see comments regarding observation cutoff, copying configuration data and increments to COM.

github-actions · 2023-08-18T12:42:49Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

github-actions · 2023-08-18T13:49:53Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

aerorahul

A quick review on the yaml copying. Thanks for including the changes in config.com.

I am working with @CoryMartin-NOAA on the window specification, but unfortunately I have little availability at the moment to work on this new requirement. I'll get back to it ASAP.

aerorahul · 2023-08-18T14:01:38Z

ush/python/pygfs/task/land_analysis.py

@@ -336,6 +339,15 @@ def finalize(self) -> None:
        statfile = os.path.join(self.task_config.COM_LAND_ANALYSIS, f"{self.task_config.APREFIX}landstat.tgz")
        self.tgz_diags(statfile, self.task_config.DATA)

+        logger.info("Copy full YAML to COM")
+        src = os.path.join(self.task_config['DATA'], f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")


Suggested change

src = os.path.join(self.task_config['DATA'], f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")

src = os.path.join(self.task_config['DATA'], f"{self.task_config['RUN']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")

Please don't use CDUMP anymore.

Also, isn't this already defined as self.task_config.APREFIX? Why do we need to reconstruct that again here?

Thanks @aerorahul for quick review. Good catch, and I made the changes as suggested.

aerorahul · 2023-08-18T14:01:59Z

ush/python/pygfs/task/land_analysis.py

@@ -336,6 +339,15 @@ def finalize(self) -> None:
        statfile = os.path.join(self.task_config.COM_LAND_ANALYSIS, f"{self.task_config.APREFIX}landstat.tgz")
        self.tgz_diags(statfile, self.task_config.DATA)

+        logger.info("Copy full YAML to COM")
+        src = os.path.join(self.task_config['DATA'], f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")
+        dest = os.path.join(self.task_config.COM_CONF, f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")


Suggested change

dest = os.path.join(self.task_config.COM_CONF, f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")

dest = os.path.join(self.task_config.COM_CONF, f"{self.task_config['RUN']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")

Also, isn't this already defined as self.task_config.APREFIX? Why do we need to reconstruct that again here?

Made the changes as suggested.

github-actions · 2023-08-18T14:41:08Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

aerorahul · 2023-08-22T20:36:49Z

@jiaruidong2017
Can I suggest you remove this +/- 1sec business from this PR and then we can merge this PR.
Meanwhile, I will work on a better solution for the window manipulation in the yaml rather than hard-wiring it in the python code.
Please let me know when you have this change.
Thanks

github-actions · 2023-08-23T00:34:52Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

jiaruidong2017 · 2023-08-23T00:36:10Z

@jiaruidong2017 Can I suggest you remove this +/- 1sec business from this PR and then we can merge this PR. Meanwhile, I will work on a better solution for the window manipulation in the yaml rather than hard-wiring it in the python code. Please let me know when you have this change. Thanks

@aerorahul I removed the changes for the window manipulations. Thanks for your suggestions and your efforts.

github-actions · 2023-08-23T00:45:00Z

Link to ReadTheDocs sample build for this PR can be found at:
https://global-workflow--1797.org.readthedocs.build/en/1797

aerorahul

Looks good.
Thank you for accepting the suggestions.
I will get the solution for windows in ASAP.
I will reach out to get a setup to test the changes.

aerorahul · 2023-08-23T01:25:40Z

@WalterKolczynski-NOAA
The changes in this PR are not covered in the tests. So I don't think there is a reason to run the CI on this PR.
This can be merged as is.

WalterKolczynski-NOAA · 2023-08-23T01:27:07Z

Do the title/description need to be updated now that the window is being handled elsewhere?

jiaruidong2017 · 2023-08-23T02:27:26Z

Do the title/description need to be updated now that the window is being handled elsewhere?

@WalterKolczynski-NOAA I updated the title and descriptions for your review. I prefer to keep the descriptions of the DA window manipulation there for future references.

Shift window_begin 1 second backward

bdd67fc

Copy the generated YAML file from initialize to the COM Copy the increment files to the COM/

Fix python norm issue

5dd9539

CoryMartin-NOAA reviewed Aug 14, 2023

View reviewed changes

Add 1S to the wind lenght and remove a space.

d38d227

jiaruidong2017 requested a review from CoryMartin-NOAA August 15, 2023 01:14

Set the window lenght 6 hours + 2 seconds to make DA window

0060e35

centering at the cycle time.

Change the window length.

a56a834

Merge branch 'develop' into feature/window_shift

69f1d67

Update GDASApp commit hash.

35104d9

Update the window length to 6 hr 1 sec.

81cd817

CoryMartin-NOAA approved these changes Aug 16, 2023

View reviewed changes

aerorahul reviewed Aug 16, 2023

View reviewed changes

ush/python/pygfs/task/land_analysis.py Show resolved Hide resolved

aerorahul requested changes Aug 16, 2023

View reviewed changes

Merge branch 'develop' into feature/window_shift

a5e36be

create a COM_CONF_TMPL and copy YAML file into COM_CONF.

4e22b5a

jiaruidong2017 requested a review from aerorahul August 18, 2023 13:59

aerorahul requested changes Aug 18, 2023

View reviewed changes

Address the reviewer comments.

696fbf1

jiaruidong2017 requested a review from aerorahul August 21, 2023 17:15

Revert the changes for window manipulation.

c32b1b4

Merge branch 'develop' into feature/window_shift

7c4b99c

aerorahul approved these changes Aug 23, 2023

View reviewed changes

WalterKolczynski-NOAA approved these changes Aug 23, 2023

View reviewed changes

jiaruidong2017 changed the title ~~Shift window_begin for loss of observations and copy some files to COM for archive~~ Manage to copy some files to COM for archive Aug 23, 2023

WalterKolczynski-NOAA changed the title ~~Manage to copy some files to COM for archive~~ Copy land increments and land DA confs to COM Aug 23, 2023

WalterKolczynski-NOAA merged commit c7fdee8 into NOAA-EMC:develop Aug 23, 2023
5 checks passed

jiaruidong2017 deleted the feature/window_shift branch August 23, 2023 12:23

	src = os.path.join(self.task_config['DATA'], f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")
	src = os.path.join(self.task_config['DATA'], f"{self.task_config['RUN']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")

	dest = os.path.join(self.task_config.COM_CONF, f"{self.task_config['CDUMP']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")
	dest = os.path.join(self.task_config.COM_CONF, f"{self.task_config['RUN']}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")

Copy land increments and land DA confs to COM #1797

Copy land increments and land DA confs to COM #1797

Conversation

jiaruidong2017 commented Aug 12, 2023 • edited Loading

github-actions bot commented Aug 12, 2023

github-actions bot commented Aug 12, 2023

CoryMartin-NOAA commented Aug 14, 2023

jiaruidong2017 commented Aug 14, 2023

CoryMartin-NOAA commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barlage Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

aerorahul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2023

aerorahul commented Aug 22, 2023

github-actions bot commented Aug 23, 2023

jiaruidong2017 commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

aerorahul left a comment

Choose a reason for hiding this comment

aerorahul commented Aug 23, 2023

WalterKolczynski-NOAA commented Aug 23, 2023

jiaruidong2017 commented Aug 23, 2023

jiaruidong2017 commented Aug 12, 2023 •

edited

Loading

barlage Aug 15, 2023 •

edited

Loading

aerorahul Aug 16, 2023 •

edited

Loading

aerorahul Aug 16, 2023 •

edited

Loading

aerorahul Aug 17, 2023 •

edited

Loading